Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add GRPC Reflection service #15

Merged
merged 2 commits into from
Jan 23, 2025
Merged

Conversation

igor-vovk
Copy link
Contributor

Small PR that adds GRPC reflection support.

Makes it easier to use tools like Postman or grpcurl to see what APIs server exposes and to call them.

I know that the state can be seen using port 9000, but it's easier to explain what XDS server does to others by showing exact XDS APIs that are exposed, otherwise it's a little bit of a blackbox and requires proto files to call it.

@igor-vovk igor-vovk changed the title Add GRPC reflection service Add GRPC Reflection service Jan 8, 2025
@igor-vovk
Copy link
Contributor Author

Hi @whs! What do you think about this PR?

@whs
Copy link
Member

whs commented Jan 16, 2025

Hi,

Sorry I didn't have time to review this yet. Generally the idea of adding reflection is ok to me, but may need a security consideration aspect. If I have time I'll review this PR in more details

Thank your for the pull request

main.go Outdated
@@ -76,6 +77,7 @@ func main() {
}()

grpc_health_v1.RegisterHealthServer(grpcServer, healthServer)
reflectionservice.Register(grpcServer)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have option to switch this on/off and default to off so the behavior is still the same? Environment variable should be perfect choice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea! What do you think about ENABLE_GRPC_REFLECTION env variable? Added commit with env check

@suparitlmwn
Copy link

LGTM

@whs whs merged commit ee77a18 into wongnai:master Jan 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants